Currently PR_NewLogModule() inserts new modules into a linked-list which isn't protected by a lock. This makes the call potentially racy opening up to the rare possibility that a new module is created but not inserted in the list as the head is overwritten by a module created simultaneously. _PR_InitLog() which walks the module list can find himself in a similar situation (the module list being changed while it's already walking it) thus not walking over all the nodes. Both use-cases should be protected by a lock to ensure they're thread-safe.

This patch moves the initialization of PR_Log to before NSPR init inserts log modules.

This patch makes the _PR_LOCK macros reusable so that they can be used for another lock.

This patch adds a lock for guarding access to the logModules global.

And here we finally guard access to logModules when modifying its value or iterating over its entries.

Is PR_NewLogModule() designed to be called in the main thread only?

(In reply to JW Wang [:jwwang] from comment #5) > Is PR_NewLogModule() designed to be called in the main thread only? It should be possible to call it from any thread; however until this bug is fixed doing so is racy.

(In reply to Eric Rahm [:erahm] from comment #4) > And here we finally guard access to logModules when modifying its value or > iterating over its entries. Maybe it's overkill but wouldn't it be better to create a few thread-safe functions to manipulate logModules (e.g. insert a module, iterate over existing modules and perform an action, etc...) rather than modifying the existing uses? It would probably be more verbose but also cleaner and less prone to future mistakes.

(In reply to Gabriele Svelto [:gsvelto] from comment #7) > (In reply to Eric Rahm [:erahm] from comment #4) > > And here we finally guard access to logModules when modifying its value or > > iterating over its entries. > > Maybe it's overkill but wouldn't it be better to create a few thread-safe > functions to manipulate logModules (e.g. insert a module, iterate over > existing modules and perform an action, etc...) rather than modifying the > existing uses? It would probably be more verbose but also cleaner and less > prone to future mistakes. I think that would be an excellent follow up, particularly when we add more log module usage in bug 1074149. For now I'd like to keep this patch set as simple as possible.

Firefox no longer uses NSRP logging. I'll leave this bug open as it's still an issue in NSPR.

Lets just close this, odds are the patches still apply but it's gotten no traction.